Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: disable v8 snapshots #585

Merged
merged 1 commit into from
Jan 26, 2015
Merged

Conversation

bnoordhuis
Copy link
Member

Snapshots speed up start-up by a few milliseconds but are potentially
dangerous because of the fixed hash seed that is used for strings and
dictionaries, making collision denial-of-service attacks possible.

Release builds on iojs.org have snapshots disabled but source builds
did not, until now.

The risk for individual source builds is low; the binary gets a random
32 bits hash seed that should be hard to guess by an external attacker.

It's when binaries are distributed by, for example, a distro vendor
that the fixed hash seed becomes a vulnerability, because then it's
possible to target a large group of people at once.

People that really need the faster start-up time can use the new
--with-snapshot configure flag.

R=@piscisaureus

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/106/

@rvagg
Copy link
Member

rvagg commented Jan 24, 2015

+1 from me on this!

@piscisaureus
Copy link
Contributor

@bnoordhuis This does the job only for unix. But with that remark, lgtm.
(I'm still pondering on the best way to unify windows and unix in their use of configure flags).

@bnoordhuis
Copy link
Member Author

@piscisaureus vcbuild.bat ends up running configure, doesn't it? I can update it to rename the nosnapshot option but that would be a blind change.

@piscisaureus
Copy link
Contributor

@bnoordhuis I'll test it for ya

@rvagg
Copy link
Member

rvagg commented Jan 24, 2015

@piscisaureus
Copy link
Contributor

@bnoordhuis Looking good.

@jbergstroem
Copy link
Member

👍 from me as well. This should have been the default all along.

@rvagg
Copy link
Member

rvagg commented Jan 24, 2015

this is almost a breaking change because --without-snapshot will cause configure to fail, I wonder if we should add back in support for that option that makes it a noop?

@jbergstroem
Copy link
Member

@rvagg that specific comment has also been nagging me for a while. It's pretty common practise for flags in "autoconf"-like environments to always allow both variations {with,}out, enable/disable, etc - but showcasing a way to toggle defaults (--with-snapshot would be shown if --without is default). And if we're really changing this, I think snapshot should really be --enable-snapshot since you don't pass anything to it.

Is it possibly worth to redo most of these flags at some point?

@bnoordhuis
Copy link
Member Author

I removed the --without-snapshot flag from the Makefiles, PTAL.

this is almost a breaking change because --without-snapshot will cause configure to fail, I wonder if we should add back in support for that option that makes it a noop?

shrug Flags have come and gone before. I can add a no-op --without-snapshot if you feel strongly about it.

@rvagg
Copy link
Member

rvagg commented Jan 26, 2015

Google "./configure --without-snapshot", over 20k results.

I'm a little ashamed to say that I do feel strongly about this, a noop to prevent surprises and break scripted builds would be my preference here.

@bnoordhuis
Copy link
Member Author

@piscisaureus @rvagg Updated, PTAL.

@rvagg
Copy link
Member

rvagg commented Jan 26, 2015

lgtm

Snapshots speed up start-up by a few milliseconds but are potentially
dangerous because of the fixed hash seed that is used for strings and
dictionaries, making collision denial-of-service attacks possible.

Release builds on iojs.org have snapshots disabled but source builds
did not, until now.

The risk for individual source builds is low; the binary gets a random
32 bits hash seed that should be hard to guess by an external attacker.

It's when binaries are distributed by, for example, a distro vendor
that the fixed hash seed becomes a vulnerability, because then it's
possible to target a large group of people at once.

People that really need the faster start-up time can use the new
--with-snapshot configure flag.

PR-URL: nodejs#585
Reviewed-By: Bert Belder <bertbelder@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
@jbergstroem
Copy link
Member

lgtm too

@bnoordhuis bnoordhuis closed this Jan 26, 2015
@bnoordhuis bnoordhuis deleted the disable-v8-snapshots branch January 26, 2015 23:55
@bnoordhuis bnoordhuis merged commit 4f68369 into nodejs:v1.x Jan 26, 2015
orangemocha pushed a commit to orangemocha/node that referenced this pull request Jun 9, 2015
Snapshots speed up start-up by a few milliseconds but are potentially
dangerous because of the fixed hash seed that is used for strings and
dictionaries, making collision denial-of-service attacks possible.

Release builds on iojs.org have snapshots disabled but source builds
did not, until now.

The risk for individual source builds is low; the binary gets a random
32 bits hash seed that should be hard to guess by an external attacker.

It's when binaries are distributed by, for example, a distro vendor
that the fixed hash seed becomes a vulnerability, because then it's
possible to target a large group of people at once.

People that really need the faster start-up time can use the new
--with-snapshot configure flag.

Cherry picked from bnoordhuis/io.js@4f68369
Original commit metadata below:
  PR-URL: nodejs/node#585
  Reviewed-By: Bert Belder <bertbelder@gmail.com>
  Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
  Reviewed-By: Rod Vagg <rod@vagg.org>
orangemocha pushed a commit to orangemocha/node that referenced this pull request Jul 7, 2015
Snapshots speed up start-up by a few milliseconds but are potentially
dangerous because of the fixed hash seed that is used for strings and
dictionaries, making collision denial-of-service attacks possible.

Release builds on iojs.org have snapshots disabled but source builds
did not, until now.

The risk for individual source builds is low; the binary gets a random
32 bits hash seed that should be hard to guess by an external attacker.

It's when binaries are distributed by, for example, a distro vendor
that the fixed hash seed becomes a vulnerability, because then it's
possible to target a large group of people at once.

People that really need the faster start-up time can use the new
--with-snapshot configure flag.

Cherry picked from bnoordhuis/io.js@4f68369
Original commit metadata below:
  PR-URL: nodejs/node#585
  Reviewed-By: Bert Belder <bertbelder@gmail.com>
  Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
  Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants